Major change: Rework namespace (get rid of ::config namespace again) #950
Conversation
|
Pinging @danzilio and @bastelfreak again |
|
After community modules review, went in the direction that modules team is recommending as current best practice. This is pretty big and nasty, so would appreciate some eyes to make sure I didn't massively screw something up, though I am sure there will need to be some more improvements. This will definitely be a breaking change and a big 180; the I'm not quite sure how the config class was getting Debian / Ubuntu type defaults before in the 'with defaults' context; I changed tests in that scope to the actual service and package still are private classes, but are not yet quite following the same convention, but want to tackle that separately if / after this gets merged. Followed @danzilio's first suggestion about the specs (which @hunner agreed with), namely, including all the private classes in the main class spec. For now, I still left the private classes in their own contexts. Class spec is now passing, but still need to work on the other failing tests. |
wyardley
changed the title
|
Also pulled out the redundant validations in |
| @@ -247,12 +139,12 @@ | |||
| File["${conf_dir}/conf.d"] { | |||
| purge => true, | |||
| recurse => true, | |||
| notify => Class['::nginx::service'], | |||
| notify => Class['service'], | |||
There was a problem hiding this comment.
just curious, why did you remove the ::nginx?
There was a problem hiding this comment.
That is a good question. To be honest, I think that's the result of an overzealous sed command or search / replace!
Updated and pushed, thanks for catching that.
|
ps - I'll do a rebase too |
|
This seems like a good change to make here, but why? I'm on a version of the module (in one of my environments) where setting values on the Fwiw, this also should make it simpler to migrate that data to puppet4 native hiera if we wanted. |
|
@xaque208 we figured out that throwing the warning doesn't meet the current best practices so we are removing it. |
|
Makes sense, thanks @bastelfreak. |
ghost
mentioned this pull request
Major change: Rework namespace (get rid of ::config namespace again)
Not sure how this should be accomplished, but would welcome some input, here's my first pass at restructuring things.
A few questions:
inherits ::nginxdoesn't seem to make a difference here.I thought templates would always take
@foofrom the top level namespace of the module, but with these changes, I'm getting some errors (see below).I think most of these changes will break or cause unpredictable behavior when people set nginx::config::foo in hiera... thoughts on how to warn on this or keep backwards compatibility